feat(planning): add research events and waiting-for-reply cues to the interview page - #5639

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Claude finished @kyle-sexton's task in 1m 39s —— View job

Security review complete

• Gather context (PR diff, changed files)
• Review diff for security-relevant surface
• Check for instruction-surface deletions (CLAUDE.md/AGENTS.md/rules)
• Report findings

No security issues found.

Scope reviewed: all 18 changed files (git diff origin/main...HEAD), with focus on the files most likely to carry exploitable surface:

• index.html — the new bare-#N/owner/repo#N issue-link rendering (link() helper and its two regex call sites). Both the inline [\w.-]+/[\w.-]+ repo capture and the meta.repo value used as a fallback are validated against /^[\w.-]+\/[\w.-]+$/ before being interposed into the built href, and the surrounding text has already passed through esc() (escaping &<>"') before these regexes run, so there's no way to break out of the attribute or inject markup through either path. The new waitingSince/"Research in progress" card and the confirmAccept() dialog use only esc()-wrapped or static strings in their innerHTML.
• server.py — adding research/cancel-research to REQUESTS doesn't bypass the existing record() validation: both still require id to resolve to a real question in questions.json (confirmed by the new test_08a_research_and_cancel_research_are_accepted_without_a_decision test, which asserts a 400 for an unknown id and an unknown kind). No new unauthenticated surface — /api/answer is still the same token-guarded, localhost-only endpoint, and MAX_BODY/LINE_CAP limits are untouched.
• round.py — warn_bare_issue_refs()/strings() only walk JSON values to emit a non-blocking warning; the new regexes (BARE_ISSUE_REF, CODE_SPAN) are linear, no backtracking/ReDoS risk. meta.repo is a CLI-only (round.sh/apply), trusted-local-operator input; the client still re-validates it before use as shown above.
• Schemas (event.schema.json, ops.schema.json, questions.schema.json) — additive only (research/cancel-research kinds, meta.repo, waitingSince), nothing loosened.
• skills/interview/context/surface.md — documentation changes are additive (new events/fields documented); no standing instruction, guardrail, or rule is deleted, narrowed, or softened, so the instruction-surface-deletion lens doesn't apply here.

This tool remains 127.0.0.1-only and token-guarded per server.py's existing design; nothing in this PR widens that trust boundary.

Claude finished @kyle-sexton's task in 1m 28s —— View job

Security review complete

• Gather context (git diff origin/main...HEAD, 18 changed files)
• Review diff for security-relevant surface (injection, authz, trust boundaries, secrets)
• Check for instruction-surface deletions (CLAUDE.md/AGENTS.md/rules)
• Report findings

No security issues found.

Scope: the full diff at HEAD 0fe76df (18 files, +270/-45), concentrated on the files carrying executable or trust-relevant surface:

• index.html — bare #N/owner/repo#N issue-link rendering. The link() helper and both regex call sites constrain repo to [\w.-]+\/[\w.-]+ and n to \d+ before interpolating into the built href/anchor text, and the surrounding text has already passed through esc(). No way to break out of the attribute or inject markup through either path, and S.doc.meta.repo is re-validated client-side against the same pattern even though it originates from a trusted local CLI operator, not a remote actor.
• index.html — new researchNow() toast. Interpolates the question id into the toast string, but renderToast() wraps the whole message in esc() before innerHTML, so this stays safe.
• server.py — research/cancel-research added to REQUESTS. Both still flow through record()'s existing qid not in qs check, so an unknown id is still rejected; no new unauthenticated surface, and /api/answer stays the same token-guarded, localhost-only endpoint. MAX_BODY/LINE_CAP are untouched.
• round.py — warn_bare_issue_refs()/strings(). Linear regexes (BARE_ISSUE_REF, CODE_SPAN, REPO_SLUG), no backtracking/ReDoS risk; this only emits a non-blocking warning and never gates apply. meta.repo is CLI-only, trusted-local-operator input, and the client independently re-validates it before use (see above) — not a double-trust gap.
• A prior Codex review flagged two issues against an earlier commit (6a11f78): (1) the "Research this" button rendering while a question waits on the user, and (2) warn_bare_issue_refs treating any non-empty meta.repo as configured even when malformed. Both are already fixed on this head: the talk-row logic at index.html#L1032-L1039 now special-cases st === "need" to render neither button, and warn_bare_issue_refs at round.py#L331-L333 now checks REPO_SLUG.fullmatch(...) rather than mere truthiness. Both were code-quality/correctness issues rather than security findings and are not re-reported here.
• Schemas (event.schema.json, ops.schema.json, questions.schema.json) — additive only (research/cancel-research kinds, meta.repo, waitingSince), nothing loosened.
• Instruction-surface check — skills/interview/context/surface.md changes are additive documentation (new events/fields/behavior documented); no standing instruction, guardrail, or rule is deleted, narrowed, or softened. docs/conventions/instruction-exception-register/README.md was not present in this checkout to consult directly, but the diff reads as pure addition under the consequence test, so the lens doesn't surface a finding here.

The tool remains 127.0.0.1-only and token-guarded per server.py's existing design; nothing in this PR widens that trust boundary.

Claude finished @kyle-sexton's task in 3m 25s —— View job

Code review complete

• Gather context (PR diff, changed files)
• Apply skip gate (not a skip: PR is open, ready for review, meaningful diff)
• Review changed files against CLAUDE.md / AGENTS.md and code-review lane criteria
• Post inline findings (none met the bar)
• Final summary

No new findings at this head (0fe76df).

Both issues Codex flagged on the prior revision (6a11f78) are already fixed by the latest commit, 0fe76df ("hide Research this on user holds and lint malformed meta.repo"):

• index.html's talk-row now has an explicit st === "need" branch (index.html#L1037) so a question held on the user (waitingBy: "user") no longer renders an enabled "Research this" button.
• round.py's warn_bare_issue_refs now validates meta.repo against REPO_SLUG (fullmatch on owner/repo) (round.py#L334) instead of just checking truthiness, so https://github.com/o/r or o/r/ still trigger the warning. Covered by test_bare_issue_ref_warns_when_meta_repo_is_not_an_owner_repo_slug.

Traced through the rest of the diff for correctness (not restating the author's own verification, which I did not independently re-run):

• server.py's question_states rewrite: revising is now computed directly from each question's own events entries (not propagated to children), matching the updated docstring and test_7_revising_marks_the_question_with_its_own_delivered_unhandled_decision. children is still used for the stale-cascade logic a few lines up, so it isn't dead code.
• record() in server.py correctly leaves research/cancel-research out of DECISIONS, so they don't write to r["responses"][qid] and still 400 on an unknown id — matches test_08a_research_and_cancel_research_are_accepted_without_a_decision.
• The bare-#N / owner/repo#N linking regexes in index.html's inline() (link() helper) only activate on a validated meta.repo slug and are checked after esc(), consistent with the earlier security pass on this PR.
• op_wait in round.py stamps and clears waitingSince symmetrically with the other hold fields; the "Research in progress, started …" card only reads it for st === "wait", not "need", which is correct per the design.

Scope was the 18 changed files via git diff origin/main...HEAD. Security-relevant surface was already covered by the dedicated security-review lane on this PR, so it's out of scope here.